PYTHON 5212 - Use asyncio.loop.sock_connect in _async_create_connection#2383
PYTHON 5212 - Use asyncio.loop.sock_connect in _async_create_connection#2383NoahStapp merged 10 commits intomongodb:masterfrom
Conversation
pymongo/pool_shared.py
Outdated
| _set_keepalive_times(sock) | ||
| sock.connect(sa) | ||
| sock.setblocking(False) | ||
| await asyncio.get_running_loop().sock_connect(sock, sa) |
There was a problem hiding this comment.
We need to do the same above for unix sockets as well.
There was a problem hiding this comment.
Also don't we need to add more code to emulate the connect_timeout behavior now that we're using sock.setblocking(False)? Or is the async connect timeout behavior handled at a higher level?
There was a problem hiding this comment.
Yes: we'll have to move all socket timeout behavior out of the We already do this.socket itself and into the AsyncNetworkInterface. In the middle of that now.
There was a problem hiding this comment.
Sorry, specifically for encryption Incorrect, we already handle raw async socket timeouts correctly to account for this change. The socket only needs to be nonblocking for creation._async_configured_socket raw sockets. We manage AsyncNetworkInterface timeouts at a higher level already.
There was a problem hiding this comment.
The problem is that _async_create_connection is called before AsyncNetworkInterface is ever created. So the timeout needs to be enforced here.
There was a problem hiding this comment.
I misunderstood what you meant. My next commit already addresses this with an asyncio.wait_for.
|
Scheduled every Evergreen task for maximum confidence in the correctness of the change. |
|
Encryption test failures are due to |
|
All test failures are known failures or persistent TLS PyPy issues. |
pymongo/pool_shared.py
Outdated
| return sock | ||
| except asyncio.TimeoutError as e: | ||
| sock.close() | ||
| raise socket.timeout("timed out") from e |
There was a problem hiding this comment.
Should we store the exception and carry on like we do in the sync OSError case?:
except asyncio.TimeoutError as e:
sock.close()
err = socket.timeout("timed out")
err.__cause__ = eIf not, I worry about subtle behavior differences between sync and async.
There was a problem hiding this comment.
This is a slick workaround to the lack of from e without raise.
|
|
||
| self.assertLessEqual( | ||
| sorted(latencies, reverse=True)[0], | ||
| 0.2, |
There was a problem hiding this comment.
I worry only adding 100ms will be flaky, can we make this 1 full second?
There was a problem hiding this comment.
Good call, I was slightly too optimistic about the EG host performance.
2 participants
No description provided.